Don't connect to ldap on startup #6565
Conversation
| @@ -207,7 +207,7 @@ func (c ConnWithReconnect) GetConnection() (*ldap.Conn, error) { | |||
| func (c ConnWithReconnect) ldapAutoConnect(config Config) { | |||
| l, err := c.ldapConnect(config) | |||
| if err != nil { | |||
| c.logger.Error().Err(err).Msg("autoconnect could not get ldap Connection") | |||
| c.logger.Debug().Err(err).Msg("autoconnect could not get ldap Connection") | |||
There was a problem hiding this comment.
I rather not merge this. As this will make it really hard to debug when there a real connection issues to the server.
A simple fix might be to actually just delay the initial connection until the first request is send (by the the IDM service should be up). Without having tried it, I think it might be enough to just remove the c.ldapConnect() call on line 208. Then the first call to any of the LDAP methods should trigger a new connection.
There was a problem hiding this comment.
The same code also lives again in reva. There the Logging calls are already reduced to Debug should we change that too then?
There was a problem hiding this comment.
There the Logging calls are already reduced to
Debugshould we change that too then?
Yeah, I think so.
There was a problem hiding this comment.
Looking at the code I think you are right. I changed the code accordingly.
Do you have any idea how to test this? (Except from assuming it's fixed)
There was a problem hiding this comment.
If the test suite succeeds it is already a very good indication that the delay of the initial connection doesn't break anything.
To check if the error message does no longer appear at startup. You could e.g. point the graph service to a non existing LDAP server. The error will now only be displayed once the graph service needs to access the LDAP server (e.g. during first login)
There was a problem hiding this comment.
👍 Can reproduce expected behaviour when running with not existent LDAP_URI
kobergj
force-pushed
the
RemoveLDAPErrorLog
branch
from
8c09f9e to
88fada3
Compare
Signed-off-by: jkoberg <[email protected]>
kobergj
force-pushed
the
RemoveLDAPErrorLog
branch
from
88fada3 to
eb9d2bc
Compare
|
💥 Acceptance test localApiTests-apiAntivirus-ocis failed. Further test are cancelled... |
|
Kudos, SonarCloud Quality Gate passed! |
Don't connect to ldap on startup
Reducing log level of the ldap connect error messages to debug. Corresponding reva code already logs on debug. See here: https://github.com/cs3org/reva/blob/edge/pkg/utils/ldap/reconnect.go#L206We removed the call to connect to ldap instead. It will connect on the first request now. This should remove the misleading error messages on startup
Fixes #4520